Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change how "Get Z value from project terrain" tool is presented in Mesh Editing #60709

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

JanCaha
Copy link
Contributor

@JanCaha JanCaha commented Feb 21, 2025

Description

Changed the way how Get Z value from project terrain is presented to the user according to #60512 (comment).

The Z Value and Get Z value from project terrain checkboxes are mutually exclusive, so user can either select one or the other.

Works with both Preview Transform and Apply Transform.

Clipboard 1

@github-actions github-actions bot added this to the 3.44.0 milestone Feb 21, 2025
Copy link

github-actions bot commented Feb 21, 2025

🪟 Windows builds

Download Windows builds of this PR for testing.
Debug symbols for this build are available here.
(Built from commit 1eb57e6)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit 1eb57e6)

Copy link
Contributor

@uclaros uclaros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice change, the tool now blends nicely with the Transform Mesh Vertices panel!

@DelazJ
Copy link
Contributor

DelazJ commented Feb 28, 2025

@JanCaha Thanks for the follow-up

Changed the way how Get Z value from project terrain is presented to the user according to #60512 (comment).

Hum... The comment you are referring to was instead agreeing to add the option to the Z widget 🤔

The Z Value and Get Z value from project terrain checkboxes are mutually exclusive, so user can either select one or the other.

So that means a user can enter value for X, Y and check "Get Z value from terrain" at the same time? Then, when is the Z value picked from the terrain: at the initial position (before moving xy), or after the move is done? We should clarify this in the GUI IMHO (and for docs).

Would you know why "Apply transform" is disabled in the screenshot?

Copy link
Contributor

@uclaros uclaros left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So that means a user can enter value for X, Y and check "Get Z value from terrain" at the same time?

Yes.

Then, when is the Z value picked from the terrain: at the initial position (before moving xy), or after the move is done? We should clarify this in the GUI IMHO (and for docs).

It doesn't really make much sense to get the z from the initial position, however we could clarify that on a tooltip

Would you know why "Apply transform" is disabled in the screenshot?

Apply transform is enabled after preview is clicked

Comment on lines +771 to +772
if ( vertexTransformed )
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I wonder if it is safe to skip vertices that failed the transform, or if mNewZValues needs to be matching mNewXYValues in size and we should just return false on exception.

Comment on lines +753 to +756
if ( mZFromTerrain )
{
if ( terrainProvider )
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Group those two?

    if ( mZFromTerrain && terrainProvider )
    {


try
{
point = transformation.transform( vert.x(), vert.y() );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harissou has a point, if calcX || calcY we should be transforming mNewXYValues.constLast() instead

Comment on lines 147 to 148
this method not apply new vertices to the mesh layer but only store the calculated transformation
that can be apply later with :py:func:`QgsMeshEditor.advancedEdit()`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if this is public API, but anyway, could you update these sentences while at it, please? Thanks.

   This method does not apply new vertices to the mesh layer but only stores the calculated transformation
   that can be applied later with :py:func:`QgsMeshEditor.advancedEdit()`

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants